Conversation
c90a2dc to
9f6239b
Compare
ysv
left a comment
There was a problem hiding this comment.
@ysv reviewed 3 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on masihyeganeh, metalarm10, miladz68, and TxCorpi0x).
integration-tests/modules/dex_test.go line 3058 at r1 (raw file):
// via raw bank keeper. This test asserts the fix: the second order (which would // lock frozen tokens) must be rejected. func TestFrozenTokenEscapeViaDEX(t *testing.T) {
are these 2 tests from immunify bugreport ?
they are almost equal so I would keep 1 one of them. Either integration or keeper test
x/asset/ft/keeper/keeper_dex.go line 69 at r1 (raw file):
for _, send := range actions.Send { // Validate frozen balances before sending to prevent transferring frozen tokens
does this part of code run during order execution ?
As far as I remember our plan was to make sure that if order is inside orderbook it will be guaranteed executable when matched. And I think this check brakes such logic
Lets discuss
integration-tests/modules/dex_test.go line 3230 at r1 (raw file):
placeOrder2, ) requireT.Error(err, "Second order of 400,000 must fail: it would lock frozen tokens; available spendable is 0")
maybe it makes sense to check that address is not able to lock 100k or 10k also.
Smaller portion of frozen/locked.
Description
Reviewers checklist:
Authors checklist
This change is